Skip to content

Set PARALLEL_UPDATES to 0 for all zwave_js platforms#71626

Merged
balloob merged 1 commit intohome-assistant:devfrom
raman325:parallel
May 12, 2022
Merged

Set PARALLEL_UPDATES to 0 for all zwave_js platforms#71626
balloob merged 1 commit intohome-assistant:devfrom
raman325:parallel

Conversation

@raman325
Copy link
Copy Markdown
Contributor

@raman325 raman325 commented May 10, 2022 •
edited
Loading

Proposed change

The zwave_js integration can let the driver handle parallel-ness of platform service calls. We should just fire and go.

A separate PR will be made to allow for parallel zwave_js service calls

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New integration (thank you!)
  • New feature (which adds functionality to an existing integration)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Additional information

  • This PR fixes or closes issue: fixes #
  • This PR is related to issue:
  • Link to documentation pull request:

Checklist

  • The code change is tested and works locally.
  • Local tests pass. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the development checklist
  • The code has been formatted using Black (black --fast homeassistant tests)
  • Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

If the code communicates with devices, web services, or third-party tools:

  • The manifest file has all fields filled out correctly.
    Updated and included derived files by running: python3 -m script.hassfest.
  • New or updated dependencies have been added to requirements_all.txt.
    Updated by running python3 -m script.gen_requirements_all.
  • For the updated dependencies - a link to the changelog, or at minimum a diff between library versions is added to the PR description.
  • Untested files have been added to .coveragerc.

The integration reached or maintains the following Integration Quality Scale:

  • No score or internal
  • 🥈 Silver
  • 🥇 Gold
  • 🏆 Platinum

To help with the load of incoming pull requests:

@raman325 raman325 requested a review from a team as a code owner May 10, 2022 06:05
@probot-home-assistant
Copy link
Copy Markdown

probot-home-assistant bot commented May 10, 2022

Hey there @home-assistant/z-wave, mind taking a look at this pull request as it has been labeled with an integration (zwave_js) you are listed as a code owner for? Thanks!
(message by CodeOwnersMention)

balloob
balloob reviewed May 10, 2022
View reviewed changes
homeassistant/components/zwave_js/humidifier.py
from .discovery import ZwaveDiscoveryInfo
from .entity import ZWaveBaseEntity

PARALLEL_UPDATES = 0
Copy link
Copy Markdown
Member

@balloob balloob May 10, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought this was already the default for any async integration.

Copy link
Copy Markdown
Member

@balloob balloob May 10, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, looks like it

core/homeassistant/helpers/entity_platform.py

Line 141 in bf77c00

if parallel_updates is None and not entity_has_async_update:

Copy link
Copy Markdown
Member

@MartinHjelmare MartinHjelmare May 10, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Any async integration", no we don't check that. We check for async_update attribute. Which not all async integrations may use.

core/homeassistant/helpers/entity_platform.py

Lines 422 to 426 in bf77c00

entity.add_to_platform_start(
self.hass,
self,
self._get_parallel_updates_semaphore(hasattr(entity, "async_update")),
)

Copy link
Copy Markdown
Contributor Author

@raman325 raman325 May 10, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

right and to be more explicit, we don't rely on polling at all, it's all push, so there are no platforms with an update method, sync or async

Copy link
Copy Markdown
Member

@balloob balloob May 10, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aaah yeah, ok that default is not very good. Maybe we also set it to 0 if we don't find any update method ? (So set it to 1 if we find "update" as a method.)

Copy link
Copy Markdown
Contributor Author

@raman325 raman325 May 10, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would that still be potentially a concern if the services are synchronous?

Copy link
Copy Markdown
Member

@balloob balloob May 12, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will merge this and think about it some more.

@balloob balloob merged commit 684fe24 into home-assistant:dev May 12, 2022
@raman325 raman325 deleted the parallel branch May 12, 2022 04:42
@github-actions github-actions bot locked and limited conversation to collaborators May 13, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@balloob balloob balloob left review comments

@MartinHjelmare MartinHjelmare MartinHjelmare approved these changes

Assignees

No one assigned

Labels

cla-signed integration: zwave_js new-feature small-pr PRs with less than 30 lines.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@raman325 @balloob @MartinHjelmare @homeassistant